feat(file-safety): guard machinery in the writer, with the identity and rights refusals (U5-B, split 1/2 of #1914, #1375) - #2357

…ve (U1, issue 1375)

Split unit U1 of PR 1833. Three changes, each with a test that fails without it:
- a caller-supplied staging path is checked for location and file type before anything is written, so an arbitrary path or a symlink cannot be published onto the target;
- a failed parent-directory fsync on POSIX is reported as PostCommitDurabilityError instead of being swallowed, so a successful return never claims durability the filesystem did not grant;
- the staged file and this write's own staging directory are released before RollbackFailureError is thrown.

Focused coverage for resolveLockKey added: canonical parent directory, a dangling-link chain, and termination at the bounded depth on a two-link cycle.

…ishTarget (U1, issue 1375)

The resolver may fall back to the given path only when lstat also reports the path as absent. An EACCES or EIO failure says nothing about whether the path is a link, so falling back would publish through a link we were not allowed to inspect. Focused tests added for both branches.

… type-sound

compile failed at the unit head on three points:
- RollbackFailureError needs a string backupPath, but the throw now happens after cleanup, so the
  `string | null` narrowing was lost. The failure is now held as { error, backupPath }.
- The async lstat stand-in is built on the Stats prototype so it satisfies fsSync.Stats.
- The realpath/readlink mocks are typed to the real signatures; the readlink mock answers once
  because only the link path is read.

tsc clean, 50 tests pass, ESLint --max-warnings=0 clean, no suppression change.

The any usage this entry covered is gone in the rewritten file, so the count drops 4 -> 3.
eslint --prune-suppressions --max-warnings=0 confirms it.

The read tools record the observed on-disk version through task.observationRegistry, but the field
was only declared in a later unit, so at this head the call dereferences undefined and the mocked
e2e run fails on the read_file smoke tests. The registry is introduced by this unit, so the field
belongs here.

tsc clean on this unit, 11 observationRegistry tests pass, ESLint --max-warnings=0 clean.

…rtion

This unit was rebuilt from the pre-fix content source, so its copy of safeWriteText.ts
still restored the backup over a write whose commit rename had already succeeded when
the parent-directory fsync failed, and its spec asserted rmdir generically rather than
against the staging directory this write created.

Both are already settled in unit 1 (fws/u1-atomic-publish). Taking those files here
keeps the shared code byte-identical across the units, so merging the chain in order
does not overwrite unit 1's fix.

Tests: 52 passed in safeWriteText.spec.ts.

…s notice

The template literal continued on an indented source line, so the model-facing
output carried four leading tabs before the content. Replace the indented newline
with an explicit escape so the notice and the content are separated by a single
newline and nothing else.

Local note: readFileTool.spec.ts cannot run in this worktree (isbinaryfile is not
resolvable from either node_modules here); CI covers it.

safeWriteJson rethrows RollbackFailureError and the telemetry callers record only
error.message, so the generic wrapper message lost the filesystem errno text of the
publish failure. Include the publish error message while keeping the rollback context
(publishError, rollbackError, backupPath) unchanged.

Tests: 52 passed in safeWriteText.spec.ts, ESLint clean with --max-warnings=0.

… rollback failure

The assertion accepted any rmdir argument, so a regression that removed a different
directory still passed. Read this write's own staging directory from fsSync.mkdirSync
and assert that exact path.

Tests: 52 passed in safeWriteText.spec.ts, ESLint clean with --max-warnings=0.

…the Actions queue, no source change

The dump path was the fixed sibling <target>.acl.tmp. A pre-existing user file at that
path is unlinked by the failed-save branch and by both cleanup paths, and two concurrent
writes to the same target share one dump, so one write can restore or delete the other's.
Use the per-write unique name like the staging file.

Tests: 52 passed in safeWriteText.spec.ts, ESLint clean with --max-warnings=0.

…eturned

A slice that starts after line 1 and reaches EOF is not truncated, so the branch said
"The file was read in full" even though the earlier lines were omitted. Name the slice
start when the read did not begin at line 1, and add a focused test for that case.

Local note: readFileTool.spec.ts cannot run in this worktree (isbinaryfile is not
resolvable from either node_modules here) and ESLint cannot resolve its config here;
CI covers both.

…opy backup)

Pre-merge review flagged the publication path as non-atomic when backup:true: it renamed the existing target to a
backup name and only then renamed the staged file into place, so the canonical path was absent for the whole commit
window - readers saw a missing file, and a concurrent writer could create a new target that a later rollback would
destroy.

The backup is now a copy, never a move:
- the destination is created with openSync(backupPath, "wx", 0o600) before any content exists at it, so no content
  ever sits at a path whose mode fs.copyFile chose (it uses the platform creation mask subject to umask on some
  platforms, the source's mode and read-only attribute on others);
- chmod 0o600 after the copy clears a copied read-only attribute (Windows) that would otherwise make the fsync open
  fail with EACCES, and keeps a backup of a permissive file private;
- the copy is fsynced, then deleted once the commit rename has published; nothing is ever renamed back.

safeWriteText also rejects a staging file that is the target itself (same inode/device), which would otherwise let
a caller-supplied tempPath silently alias the target.

Tests: the safeWriteText spec is replaced with the copy-model spec plus a new integration spec that runs against the
real filesystem (no fs mocks) - success publishes and leaves no residue, and a directory target is rejected with the
payload bytes unchanged. The safeWriteJson spec moves from the three-rename/rollback model to the copy model, and the
lock-key spec accounts for the extra lstat the aliasing guard performs.

83 passed / 1 skipped across safeWriteText.spec + integration spec + safeWriteJson + safeWriteJson.lockKey; tsc and
eslint clean, no suppression drift.

Pre-merge review flagged that the write path trusts an unvalidated symlink target: safeWriteText resolves an
existing link to its referent and safeWriteJson then merges and publishes onto that referent, so a repository that
plants its project settings file as a link elsewhere receives the write at the linked path.

safeWriteJson now accepts confineTo. The check runs on the resolved publish target, after resolvePublishTarget and
before the merge read and before anything is staged, so a rejected write takes no lock and leaves no residue. Both
sides are canonicalized the same way: the scope is resolved through symlinks (walking to the nearest existing
ancestor when it does not exist yet, and rethrowing anything other than ENOENT), and the target is canonicalized
with the same helper because a target that does not exist yet still carries the alias components of the path it was
given.

Callers that picked a path from a known scope pass that scope; a caller that does not declare one keeps the current
behaviour. The MCP settings wiring that uses this is on the async-save series (Zoo-Code-Org#1403).

Tests: an out-of-scope path is rejected on every platform with no lock and no staged file; a planted link escaping
the project is rejected and the linked file keeps its bytes; a link inside the project still publishes to the
referent; and a scope declared through a symlinked directory that does not exist yet is accepted.

The staging-identity guard compared the caller-staged file with the target through fs.lstat(targetPath).catch(() =>
null). Any error other than ENOENT - EACCES, ENOTDIR, ELOOP - was therefore indistinguishable from "there is no
target", so a staging path that is a hard link of the target would pass the identity check, reach the commit rename,
and let the failure handler unlink the only copy of the content.

Only ENOENT now yields "no target"; anything else fails closed with a StagingPathError before anything is opened,
staged or renamed. Test: the staging path resolves to a hard link of the target (same ino/dev) and the target lstat
rejects with EACCES; the write is rejected with the comparison error and neither openSync nor rename is called.

The staging-identity guard read fs.Stats.ino/dev as JS numbers. On NTFS and ReFS those identifiers can exceed
Number.MAX_SAFE_INTEGER, and the rounding makes two different files look identical - a valid caller-staged file is
then rejected with StagingPathError (and a real alias could be missed). Both lstats now request { bigint: true } and
the comparison checks for bigint values before comparing them.

The spec stand-in carries bigint ino/dev, matching what lstat({ bigint: true }) returns at runtime (fs.BigIntStats is
a type-only export, so the double is documented at its single assertion).

The identity stand-in returns bigint identifiers regardless of the options, so the same-file tests could still pass if
either lstat dropped { bigint: true } - which is exactly the case that matters on NTFS/ReFS. Both tests now assert
that every identity lstat requested bigint stats.

The calls are filtered by their options rather than by path spelling: path.resolve prefixes a drive letter to
/tmp/... on Windows, so a path filter would only see one of the two reads on that platform.

…served

On win32 the DACL of an existing target is saved before the commit rename so it can be reapplied afterwards. Two
paths silently skipped that step and still published:
- icacls /save failed (saved === false): the dump is cleaned up and the rename proceeds, so the new file inherits
  different access rights;
- fs.access(targetPath) failed with something other than ENOENT (EACCES, ...): the catch treated "cannot check" as
  "target absent" and skipped DACL handling entirely.

Both now report through a new onWarning sink (default console.warn): the write still proceeds - a missing or failing
icacls must not leave the user unable to save, which is the documented fallback - but the caller is told the
replacement may inherit different access rights instead of discovering it later.

Tests: icacls save failure still commits the write and yields exactly one access-rights warning; an EACCES on the
target yields the could-not-check warning and no icacls call. Verified as real regression tests - neutralizing the two
warn calls makes both fail. 272 passed / 4 skipped locally; tsc and eslint clean.

onWarning is documented as the sink for non-fatal safety notices, but delivery was not isolated from the write: an
onWarning callback that threw propagated to the outer failure handler before fs.rename, turning a non-fatal notice into a
failed save. The warn binding now catches callback failures and logs them. The restore-failure notice is routed through
the same binding so a caller supplying onWarning receives it. Same fix as the u6 unit, kept aligned across the series.

Local: safeWriteText 58/58 green; eslint clean, no suppression growth.

proper-lockfile creates ${lockKey}.lock beside the lock key, and the key is the
symlink referent. A repository that plants .roo/mcp.json -> ~/.ssh/config
therefore made a confined write create a lock directory OUTSIDE the declared
scope, and when that directory was not writable the caller got a
lock-acquisition error after up to five retries instead of
ConfinedPathEscapeError. The confinement check now runs on the lock key before
acquireFileLock and is repeated on the resolved publish target inside the lock
(a peer writer may have moved the referent in between); both checks share
_assertWithinScope so they canonicalize identically.

Ported across the file-safety unit series so every unit that declares a scope
carries the same guarantee (same fix as fws/u3-observation-completeness).

Series alignment for the two review findings fixed on fws/u6-apply-patch-wiring:

1. safeWriteText's warn wrapper could not catch a rejection from an async
   onWarning sink - TypeScript accepts a value-returning callback where a void one
   is expected - so the rejected promise was left unhandled, which under Node's
   default mode can end the process after a write that already succeeded. The
   wrapper now attaches a catch handler without awaiting (awaiting would let
   warning delivery delay a committed write, or stall it on a hung sink) and
   reports the rejection through the fallback sink.

2. safeWriteJson created the target's parent directory BEFORE the preflight
   confinement check, so a confined write to an out-of-scope path with a missing
   parent still created a directory outside confineTo. resolveLockKey and the check
   need no directory to exist, so the order is now lock key, confinement, mkdir;
   the in-lock check on the resolved publish target stays.

…ith doUnmock

- safeWriteText.integration.spec: the backup:true case bails out in the backup step (copyfile
  over a directory), so the commit rename never runs and a regression in commit-failure cleanup
  would pass. Added the backup:false case, which is the shape that reaches Step 4: the rename of
  the staging file over the target directory fails and the staging file must be cleaned up.
- safeWriteJson.test: the finally blocks removed a vi.doMock registration with vi.unmock, which
  vitest hoists to the top of the file, so the cleanup did not undo the registration where it
  was written. Both sites now use vi.doUnmock, the non-hoisted counterpart.

Verified the two paths are distinct by calling safeWriteText against a real directory target:
backup:true fails at copyfile, backup:false fails at rename of the staging file.
Local: 29 passed / 4 skipped across the two specs; eslint clean on both files.

Task.ts initializes the registry per Task, but nothing read it off a real Task: the registry
tests exercise standalone instances and ReadFileTool's spec injects its own double, so the
wiring itself was unverified - a Task that never built a registry, or shared one across
Tasks, would pass every existing test while silently dropping read observations.

The new test constructs a real Task and asserts observationRegistry is an ObservationRegistry,
that a second Task gets a different instance, and that an observation recorded on one Task is
invisible to the other (parent and subtask authority over a file must stay separate).

Local: Task.dispose.test.ts 15 passed; eslint clean; no new tsc diagnostics in this file.

Security Boundaries: safeWriteJson resolves the publish target through realpath
before staging beside it, so a repository that ships .roo/mcp.json as a symlink to
a file OUTSIDE the workspace has that outside file replaced as soon as the user
edits a project MCP setting, deletes a server, or toggles tool always-allow. The
three project-capable write sites passed no confinement option, and the base
implementation renamed the link itself, so the external write is introduced here.

confineForMcpWrite(source) returns the provider cwd (falling back to
getWorkspacePath) for project writes and undefined for global ones - global
settings live in the user's own settings directory, which is deliberately not
confined. All three sites now pass it, so the write fails closed with
ConfinedPathEscapeError instead of touching the referent. This is the same change
as 256091d on fws/u3-observation-completeness (Zoo-Code-Org#1912); the unit branches are not
cumulative, so every branch that carries the writer needs its own copy.

Regression Evidence: the per-Task registry now fails closed on teardown too -
disposeOnce() clears it, so a disposed Task still reachable through a
parent/subtask reference cannot hand a token captured before teardown to a
guarded write. Two McpHub tests pin the call sites (a project-scoped allowlist
write carries confineTo = workspace root, a global one does not) and a
Task.dispose test pins the clear. The two-real-Task registry test added in
87986e9 already covers the distinctness the checklist row asked for.

Local: vitest McpHub + Task.dispose 89 passed. Negative controls: helper mutated
to return undefined -> project confine test fails; clear() commented out ->
disposal test fails; both restore green. tsc --noEmit 50 errors before and after
the change (unchanged branch baseline). eslint 0 errors / 0 warnings on all four
files, no suppression entry added or increased.

…every MCP confine site

Lifecycle Resource Cleanup: disposeOnce() cleared the registry, but a read that was
already awaiting fs.readFile when disposal began resumed afterwards and recorded the
file again - the disposed Task was authoritative once more, and it kept doing the new
post-read stat work for a turn that will never be acted on. ObservationRegistry.close()
now drops the entries AND refuses later observations, Task.disposeOnce() calls close(),
and both ReadFileTool paths skip the post-read stat + observe work when task.abort is
set (checked before the awaited stat, not after it).

Regression Evidence: the confinement fix touched three call sites but only
toggleToolAlwaysAllow was pinned. updateServerTimeout and deleteServer now have a
project case (confineTo = workspace root) and a global case (confineTo undefined) each;
the connection double is shared at describe scope so all four sites use one typed
fixture. safeWriteJson is mocked in this file, so a lower-level writer test cannot see
a missing MCP argument - each call site has to be pinned here.

Tests: 2 closure tests (close drops entries and reports closed; observe is a no-op after
close), 2 aborted-task tests (native and legacy path record nothing), 4 call-site
confinement tests, and the disposal test extended to assert the registry stays empty
when a late observation arrives.

Local: 203 passed across readFileTool, McpHub, Task.dispose and observationRegistry.
Negative controls, each mutate -> test fails -> restore -> passes: ReadFileTool guard
neutered, ObservationRegistry closure guard neutered (fails both the registry spec and
the Task disposal test), and close() downgraded to clear() (fails the disposal test).
tsc --noEmit 50 errors before and after (unchanged branch baseline, 0 in touched files).
eslint 0 errors / 0 warnings across all 8 touched files; no suppression entry changed.

The abort guard sat before the post-read fs.stat, so a cancellation that
lands while that stat is in flight still recorded an observation - for a
run that will never act on the read, and into a registry that disposal has
already retired. Both paths now re-check task.abort after the await and
before observe(); the read itself still answers the model.

Two tests flip task.abort inside the pending post-read stat, one per path,
and assert observe() was never called and the registry stayed empty.